Conversation
…sume and integrate into Quick Actions panel
WalkthroughThis PR introduces an auto-proceed feature that automatically advances scenes after a configurable delay, with pause/resume controls accessible via a new Quick Actions sidebar panel. The implementation extends config schema with ChangesAuto-Proceed Feature Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Deploying demo-time with
|
| Latest commit: |
b387f95
|
| Status: | 🚫 Build failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/vscode-extension/src/services/DemoRunner.ts (1)
453-467: 💤 Low valueUn-awaited recursive call follows existing pattern but could cause subtle races.
The recursive
DemoRunner.start(item)call at line 465 is not awaited, matching the existing pattern at lines 491-494. While consistent, this means the outerawait commands.executeCommand(COMMAND.start)(line 259) resolves before the looped iteration completes. If rapid user actions occur during the brief window, state could race.Consider awaiting the recursive call for safety:
Proposed fix
executingFile.demo = []; await DemoRunner.setExecutedDemoFile(executingFile); - DemoRunner.start(item); + await DemoRunner.start(item); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode-extension/src/services/DemoRunner.ts` around lines 453 - 467, The auto-loop path performs a non-awaited recursive call to DemoRunner.start(item), which can allow the outer caller to proceed before the restarted run completes and cause race conditions; update this block to await the recursive restart (await DemoRunner.start(item)) and ensure any surrounding callers handle the returned promise accordingly (the relevant symbols are DemoRunner.start, DemoRunner.setExecutedDemoFile, and the fileDemo?.autoLoop branch) so state is fully updated before control returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode-extension/src/panels/QuickActionsPanel.ts`:
- Around line 11-16: QuickActionsPanel.register currently creates a TreeView
(QuickActionsPanel.treeView) but doesn't add the returned disposable to the
extension singleton's subscriptions; change register so after creating the
TreeView you push the disposable into Extension.getInstance().subscriptions (and
keep creating QuickActionsPanel.quickActionsProvider as-is) to ensure cleanup on
deactivation; apply the same subscription fix to DemoPanel and ResourcesPanel
where createTreeView is used.
In `@packages/common/src/utils/demoNormalizer.ts`:
- Line 15: demoConfigToActConfig currently omits the autoLoop field causing loss
when converting legacy configs; update demoConfigToActConfig to copy autoLoop
from the source config into the returned act config (the same key
normalizeActConfig maps), e.g. set autoLoop: config.autoLoop (or fallback to
existing default behavior) so the autoLoop setting is preserved through legacy →
v3 conversion.
---
Nitpick comments:
In `@apps/vscode-extension/src/services/DemoRunner.ts`:
- Around line 453-467: The auto-loop path performs a non-awaited recursive call
to DemoRunner.start(item), which can allow the outer caller to proceed before
the restarted run completes and cause race conditions; update this block to
await the recursive restart (await DemoRunner.start(item)) and ensure any
surrounding callers handle the returned promise accordingly (the relevant
symbols are DemoRunner.start, DemoRunner.setExecutedDemoFile, and the
fileDemo?.autoLoop branch) so state is fully updated before control returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d129f991-a04c-4c81-9588-6b3a48796447
📒 Files selected for processing (9)
apps/vscode-extension/package.jsonapps/vscode-extension/src/constants/ContextKeys.tsapps/vscode-extension/src/extension.tsapps/vscode-extension/src/panels/QuickActionsPanel.tsapps/vscode-extension/src/providers/QuickActionsTreeviewProvider.tsapps/vscode-extension/src/services/DemoRunner.tspackages/common/src/constants/Command.tspackages/common/src/models/Demos.tspackages/common/src/utils/demoNormalizer.ts
| public static register() { | ||
| QuickActionsPanel.quickActionsProvider = new QuickActionsTreeviewProvider(); | ||
| QuickActionsPanel.treeView = window.createTreeView('demo-time-quick-actions', { | ||
| treeDataProvider: QuickActionsPanel.quickActionsProvider, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the Quick Actions TreeView is disposed/subscribed via ExtensionContext.
rg -n --type=ts "createTreeView\\(|QuickActionsPanel\\.register\\(|subscriptions\\.push\\(|\\.dispose\\(" apps/vscode-extension/srcRepository: estruyf/vscode-demo-time
Length of output: 9170
🏁 Script executed:
sed -n '45,70p' apps/vscode-extension/src/extension.tsRepository: estruyf/vscode-demo-time
Length of output: 769
🏁 Script executed:
sed -n '1,100p' apps/vscode-extension/src/extension.ts | head -50Repository: estruyf/vscode-demo-time
Length of output: 1568
🏁 Script executed:
cat -n apps/vscode-extension/src/services/Extension.ts | head -80Repository: estruyf/vscode-demo-time
Length of output: 2326
🏁 Script executed:
sed -n '235,250p' apps/vscode-extension/src/panels/DemoPanel.tsRepository: estruyf/vscode-demo-time
Length of output: 515
🏁 Script executed:
sed -n '210,235p' apps/vscode-extension/src/panels/DemoPanel.tsRepository: estruyf/vscode-demo-time
Length of output: 683
🏁 Script executed:
grep -n "public static register" apps/vscode-extension/src/panels/DemoPanel.tsRepository: estruyf/vscode-demo-time
Length of output: 100
🏁 Script executed:
sed -n '18,45p' apps/vscode-extension/src/panels/DemoPanel.tsRepository: estruyf/vscode-demo-time
Length of output: 886
🏁 Script executed:
sed -n '180,215p' apps/vscode-extension/src/panels/DemoPanel.tsRepository: estruyf/vscode-demo-time
Length of output: 830
🏁 Script executed:
sed -n '1,40p' apps/vscode-extension/src/panels/ResourcesPanel.tsRepository: estruyf/vscode-demo-time
Length of output: 697
Subscribe the TreeView to extension lifecycle via the Extension singleton.
createTreeView returns a disposable resource that should be registered with Extension.getInstance().subscriptions to ensure proper cleanup on extension deactivation, consistent with command and provider registration patterns used throughout the codebase.
The proposed refactor's approach of adding a context parameter doesn't align with this extension's singleton pattern. Instead, add the subscription using the Extension class's existing accessor:
Corrected refactor
public static register() {
QuickActionsPanel.quickActionsProvider = new QuickActionsTreeviewProvider();
QuickActionsPanel.treeView = window.createTreeView('demo-time-quick-actions', {
treeDataProvider: QuickActionsPanel.quickActionsProvider,
});
+ Extension.getInstance().subscriptions.push(QuickActionsPanel.treeView);
}Note: This same issue affects DemoPanel and ResourcesPanel, which also create TreeViews without subscribing them.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static register() { | |
| QuickActionsPanel.quickActionsProvider = new QuickActionsTreeviewProvider(); | |
| QuickActionsPanel.treeView = window.createTreeView('demo-time-quick-actions', { | |
| treeDataProvider: QuickActionsPanel.quickActionsProvider, | |
| }); | |
| } | |
| public static register() { | |
| QuickActionsPanel.quickActionsProvider = new QuickActionsTreeviewProvider(); | |
| QuickActionsPanel.treeView = window.createTreeView('demo-time-quick-actions', { | |
| treeDataProvider: QuickActionsPanel.quickActionsProvider, | |
| }); | |
| Extension.getInstance().subscriptions.push(QuickActionsPanel.treeView); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/vscode-extension/src/panels/QuickActionsPanel.ts` around lines 11 - 16,
QuickActionsPanel.register currently creates a TreeView
(QuickActionsPanel.treeView) but doesn't add the returned disposable to the
extension singleton's subscriptions; change register so after creating the
TreeView you push the disposable into Extension.getInstance().subscriptions (and
keep creating QuickActionsPanel.quickActionsProvider as-is) to ensure cleanup on
deactivation; apply the same subscription fix to DemoPanel and ResourcesPanel
where createTreeView is used.
| version: config.version, | ||
| timer: config.timer, | ||
| engageTime: config.engageTime, | ||
| autoLoop: config.autoLoop, |
There was a problem hiding this comment.
Preserve autoLoop in legacy → v3 conversion as well.
autoLoop is mapped in normalizeActConfig, but demoConfigToActConfig currently drops it. This causes silent loss of the setting when converting from legacy config to v3.
Proposed fix
export function demoConfigToActConfig(config: DemoConfig): ActConfig {
return {
$schema: config.$schema,
title: config.title,
description: config.description,
version: 3,
timer: config.timer,
engageTime: config.engageTime,
+ autoLoop: config.autoLoop,
scenes: config.demos.map((demo) => demoToScene(demo)),
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/common/src/utils/demoNormalizer.ts` at line 15,
demoConfigToActConfig currently omits the autoLoop field causing loss when
converting legacy configs; update demoConfigToActConfig to copy autoLoop from
the source config into the returned act config (the same key normalizeActConfig
maps), e.g. set autoLoop: config.autoLoop (or fallback to existing default
behavior) so the autoLoop setting is preserved through legacy → v3 conversion.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/common/src/models/Demos.ts (1)
54-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the time units for
autoAdvanceAfter.The
autoAdvanceAfterfield is a number but doesn't specify units. Users configuring this field won't know whether to use seconds (e.g.,5) or milliseconds (e.g.,5000). Add a JSDoc comment clarifying the expected units.📝 Suggested documentation
export interface Scene { id?: string; title: string; description?: string; moves: Move[]; icons?: Icons; notes?: Notes; disabled?: boolean; + /** + * Time in seconds to wait before automatically advancing to the next scene. + * Must be a positive number. Used with autoLoop to create automatic demo flows. + */ autoAdvanceAfter?: number; }Apply the same documentation pattern to the
Demointerface at line 66.Also applies to: 66-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/common/src/models/Demos.ts` at line 54, Add JSDoc comments clarifying the time units for the numeric autoAdvanceAfter property and the corresponding field on the Demo interface: update the declaration for autoAdvanceAfter (and the Demo interface's autoAdvanceAfter) to include a brief JSDoc like "`@param` autoAdvanceAfter - number of milliseconds to wait before auto-advancing" (or seconds if you prefer, but be explicit) so consumers know the expected units and behavior.
🧹 Nitpick comments (1)
apps/vscode-extension/package.json (1)
916-919: Spacebar keybinding is appropriately scoped but will override default behavior.The space key is bound to
toggleAutoProceedwhen in presentation mode with auto-proceed active. While thewhenclause appropriately limits the scope, users should be aware that pressing space in this context will toggle auto-proceed rather than performing default actions (e.g., scrolling in some panels). This seems intentional for quick pause/resume during auto-advancing presentations.Consider documenting this keybinding behavior in user-facing documentation or the command description so presenters know they can use space to quickly pause/resume during auto-proceed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode-extension/package.json` around lines 916 - 919, The spacebar keybinding for demo-time.toggleAutoProceed is scoped to presentation mode but overrides the default space behavior, so update the extension's user-facing docs and the command metadata to make this explicit: add or expand the description for the command "demo-time.toggleAutoProceed" in the contributes.commands section (and/or the extension README) to state that pressing Space in "demo-time.presentation && demo-time.autoProceedActive == true" will pause/resume auto-proceed, and mention any panels where default space behavior is suppressed so presenters are aware.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/common/src/models/Demos.ts`:
- Line 54: Add JSDoc comments clarifying the time units for the numeric
autoAdvanceAfter property and the corresponding field on the Demo interface:
update the declaration for autoAdvanceAfter (and the Demo interface's
autoAdvanceAfter) to include a brief JSDoc like "`@param` autoAdvanceAfter -
number of milliseconds to wait before auto-advancing" (or seconds if you prefer,
but be explicit) so consumers know the expected units and behavior.
---
Nitpick comments:
In `@apps/vscode-extension/package.json`:
- Around line 916-919: The spacebar keybinding for demo-time.toggleAutoProceed
is scoped to presentation mode but overrides the default space behavior, so
update the extension's user-facing docs and the command metadata to make this
explicit: add or expand the description for the command
"demo-time.toggleAutoProceed" in the contributes.commands section (and/or the
extension README) to state that pressing Space in "demo-time.presentation &&
demo-time.autoProceedActive == true" will pause/resume auto-proceed, and mention
any panels where default space behavior is suppressed so presenters are aware.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d9f43cd-e6f4-4cf3-91d3-b1ff705c19c9
📒 Files selected for processing (4)
apps/vscode-extension/package.jsonapps/vscode-extension/src/extension.tspackages/common/src/constants/Command.tspackages/common/src/models/Demos.ts
1 participant
Implementation for #402
Fixes #
Type of change
Checklist
Screenshots (if applicable)
Additional context
Note
Add auto-proceed functionality with pause/resume controls to demo scenes
autoAdvanceAfterper-scene config andautoLoopfile config to drive automatic scene advancement in DemoRunner.tsautoAdvanceAfter > 0, advancing to the next scene when it elapses; spacebar toggles pause/resume during presentationautoLooprequires every enabled scene to specifyautoAdvanceAfter; missing values cause an error and stop the loop rather than silently skippingMacroscope summarized b387f95.
Summary by CodeRabbit